Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove svn handling in unpacking.unpack_file #7037

Merged
merged 4 commits into from
Sep 19, 2019
Merged

Remove svn handling in unpacking.unpack_file #7037

merged 4 commits into from
Sep 19, 2019

Conversation

chrahunt
Copy link
Member

There are two code paths that lead to unpacking.unpack_file:

  1. download.unpack_url -> {download.unpack_http_url, download.unpack_file_url} -> unpacking.unpack_file
  2. wheel.build -> download.unpack_file_url -> unpacking.unpack_file

In 1, we first check whether the provided link in a VCS URL and if so we call vcs_backend.unpack directly.

In 2, we only pass in the wheel file itself - no chance the removed code is called.

@chrahunt chrahunt added type: refactor Refactoring code skip news Does not need a NEWS file entry (eg: trivial changes) labels Sep 19, 2019
@chrahunt
Copy link
Member Author

chrahunt commented Sep 19, 2019 •
edited
Loading

Actually, I think this would be called when a URL mapped to an SVN repository, but an explicit svn+ was not provided. This is not documented anywhere, so I propose to remove the behavior and direct users to use an explicit prefix.

@chrahunt
Removed unused Subversion().unpack in unpack_file
8648ff5 
`download.unpack_url` already calls `unpack_vcs_link`, it looks like
this was leftover from before that change.
@chrahunt
Remove unused argument in unpacking.unpack_file
89bbe73
@chrahunt
Remove unused functions
ea6eb1f
@chrahunt
Add news
b6f7470
@chrahunt chrahunt removed the skip news Does not need a NEWS file entry (eg: trivial changes) label Sep 19, 2019
pradyunsg
pradyunsg approved these changes Sep 19, 2019
View reviewed changes
Copy link
Member

@pradyunsg pradyunsg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yay! Dead code.

@chrahunt chrahunt marked this pull request as ready for review September 19, 2019 11:37
@chrahunt chrahunt merged commit 79ec3de into pypa:master Sep 19, 2019
@chrahunt chrahunt deleted the refactor/clean-up-unpack branch September 19, 2019 12:35
@chrahunt chrahunt mentioned this pull request Sep 21, 2019
Closed
@xavfernandez
Copy link
Member

Hmm, even if it wasn't documented, given the wide usage of pip, it's quite credible that some user somewhere was actually relying on this ^^
I would have advocated to go through the usual deprecation cycle (even for a short 3 months period).

Since it's already merged and the impacted use case is certainly quite rare and the workaround quite simple (adding the svn+ prefix), a revert seems overkill but we might want to document this workaround in the removal changelog ?

@chrahunt
Copy link
Member Author

Yes that makes sense. Currently the removal wording is:

Remove undocumented support for http:// requirements pointing to SVN repositories.

How does this sound?

Remove undocumented support for un-prefixed URL requirements pointing to SVN repositories. Users relying on this can get the original behavior by prefixing their URL with svn+ (which is backwards-compatible).

@xavfernandez
Copy link
Member

How does this sound?

Remove undocumented support for un-prefixed URL requirements pointing to SVN repositories. Users relying on this can get the original behavior by prefixing their URL with svn+ (which is backwards-compatible).

Pretty good 👍

@pradyunsg
Copy link
Member

a revert seems overkill

Reverts are cheap. I'd say we shouldn't care whether a feature is in master but rather whether it's been released, when deciding about deprecation stuff.

This is super edge-casey but yea, I'm on board to do a single-release deprecation then removal. I don't think it's needed but I'm not gonna crib or anything, if we do have it. :)

@chrahunt chrahunt mentioned this pull request Sep 22, 2019
Merged
@xavfernandez
Copy link
Member

Reverts are cheap. I'd say we shouldn't care whether a feature is in master but rather whether it's been released, when deciding about deprecation stuff.

I completely agree but since other PRs have been merged that touch the same code area, it might not be (completely) trivial.

And since we all agree that the use case is pretty niche, is undocumented and has an easy and clean workaround, I don't think it is worth it so let's move forward :)

@lock lock bot added the auto-locked Outdated issues that have been locked by automation label Oct 23, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 23, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@pradyunsg pradyunsg pradyunsg approved these changes

Assignees
No one assigned
Labels
auto-locked Outdated issues that have been locked by automation type: refactor Refactoring code
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@chrahunt @xavfernandez @pradyunsg